Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Local notifications on iOS and Android #3024

Closed
wants to merge 21 commits into from
Closed

Local notifications on iOS and Android #3024

wants to merge 21 commits into from

Conversation

roryabraham
Copy link
Contributor

@roryabraham roryabraham commented May 20, 2021 •
edited
Loading

Note: As of now this has been tested and is working well on iOS. Still need to test on Android.

Details

This PR introduces a local notification library for iOS and Android. It will display a locally-generated push notification whenever a local notification is generated on web/desktop, so:

  1. To run locally, I had to:
# This first command will reset the cache then not do anything
react-native start --reset-cache 
npm run ios
  1. When the app is in the foreground and you receive a chat notification from a chat other than the main chat.
  2. When the app is in the background but is still connected via Pusher.

    • Caveat: I'm not sure the rules for how long the app will stay connected to Pusher when it is backgrounded. During my testing, I've found some puzzling behavior...

      1. Immediately after backgrounding the app, send a message.
      2. The message is received by Pusher and a local notification displays, adds the message to Onyx, can be tapped to view the report, etc.. (new)
      3. Whenever Pusher disconnects (I still haven't figured out how to consistently know if this has happened, at least not on dev), send another message.
      4. After pusher is disconnected (and the server knows), send another message. We'll send a UA push notification and the push notification is received by the client and displayed, but the callback does not seem to execute. Not sure if this is a regression from this branch, but it seems like it hasn't worked at any point in the past. I searched LogSearch and my local logs for this line and it seems it's never logged. So it doesn't add the message to Onyx ❌ . However, the notification can be tapped, and it does seem to open the correct report (on iOS at least). When that report is opened, it reconnects to pusher and reloads messages. This is bad for offline-first.
      5. There might be some time between when pusher is connected and when it's not, during which push notifications aren't received? But they're being sent, so maybe that's just flakiness/delays with UA?
        • Sometimes, when a message like this happens and no notification displays, then you later receive + tap on a UA notification, the local notification will display afterwords?

    • TODO: discovered a bug where, if you receive push UA push notifications and they're displayed, then you tap to open that chat, then background the app, those message still appear as unread per the unread counter

Update: I think I understand the scenario in which notifications do not display in the background. This will happen when the client disconnects from the Pusher channel (happens after some undetermined amount of time with the app backgrounded), but the server still thinks they're connected. Need to dig into how this can happen and how to fix it.

Fixed Issues

Fixes (partial) https://github.com/Expensify/Expensify/issues/158830

Note that this doesn't improve push notifications in any other states (and in fact, still needs to be tested to make sure it doesn't break UA push notifications, which it may well do...) But assuming that it doesn't break UA push notifications, it will be a substantial improvement to the notification experience on iOS/Android.

Tests / QA Steps

  1. Sign into E.cash on iOS/Android in one account (A), and another account (B) on another platform.
  2. Open a chat with A and B.
  3. Send a chat from B to A. The new message should appear and there should be no notification.
  4. Open a different chat in account A.
  5. Send a message from B to A. A notification should appear for A Do not tap the notification. It should disappear after a few seconds.
  6. (iOS) Send a message from B to A. A notification should appear for A. Swipe the notification up to dismiss it. It should disappear and nothing else should happen.
  7. Send a message from B to A. A notification should appear for A. Tap the notification. The chat between A and B should open, and all three messages should immediately be present.
  8. As A, background the app.
  9. Send a message from B to A. A notification should appear for A. The app's unread counter badge should increment. Do not tap the notification. It should disappear after a few seconds.
  10. (iOS) Send a message from B to A. A notification should appear for A. The app's unread counter badge should increment Swipe the notification up to dismiss it. It should disappear and nothing else should happen.
  11. Send a message from B to A. A notification should appear for A. Tap the notification. The chat between A and B should open, and all three messages should immediately be present.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Mobile Web

Desktop

iOS

Android

@roryabraham roryabraham changed the title Rory background push notifications Handle push notifications in all app states May 20, 2021
@roryabraham roryabraham changed the title Handle push notifications in all app states [WIP] Handle push notifications in all app states May 20, 2021
@roryabraham
Rename getReportActionNotificationPayload
e5a366c
@roryabraham
Update processQueue JSDoc
2128f87
@roryabraham
Add a comment explaining why a queue is needed
4ecd1e6
@roryabraham
Add throws to JSDocs
156d31f
@roryabraham
Create cross-platform LocalNotificationService lib
f713844
@roryabraham
DRY up common notifications between platforms
88fb013
@roryabraham
Merge branch 'main' into Rory-BackgroundPushNotifications
7c5b2a2 
# Conflicts:
#	ios/ExpensifyCash.xcodeproj/project.pbxproj
@roryabraham
Reinstall node modules and pods after bad merge
df0bfac
@roryabraham
Fix AppDelegate
1453ce8
@roryabraham
Fix export and implement isVisible on mobile
1556181
@roryabraham
Merge branch 'main' into Rory-BackgroundPushNotifications
9757797 
# Conflicts:
#	ios/ExpensifyCash.xcodeproj/project.pbxproj
#	ios/Podfile.lock
#	package-lock.json
#	package.json
@roryabraham
Reinstall everything after merge
f0d5525
@roryabraham
Get onNotification callback working
a40503a
@roryabraham roryabraham changed the title [WIP] Handle push notifications in all app states Local notifications on iOS and Android Jun 18, 2021
@roryabraham roryabraham self-assigned this Jun 18, 2021
@roryabraham
Copy link
Contributor Author

roryabraham commented Jun 18, 2021 •
edited
Loading

Okay, a more comprehensive update from my testing on iOS so far today:

Ideal

App State Notification Displays? Saved to Onyx? Tapping opens to correct report?
Foreground, current report No Yes n/a
Foreground, another report Yes Yes Yes
Background, Pusher connected Yes Yes Yes
Background, pusher not connected Yes Yes Yes
Phone locked, pusher connected Yes Yes Yes
Phone locked, pusher not connected Yes Yes Yes
Closed Yes Yes Yes

Actual (production)

App State Notification Displays? Saved to Onyx? Tapping opens to correct report?
Foreground, current report No Yes n/a
Foreground, another report No ❌ Yes n/a ❌
Background, pusher connected No ❌ Yes n/a ❌
Background, pusher not connected Sometimes 🟡 No ❌ Yes
Phone locked, pusher connected No ❌ Yes n/a ❌
Phone locked, pusher not connected Sometimes 🟡 No ❌ Yes
Closed Sometimes 🟡 No ❌ Yes

Actual (in this branch)

Disclaimer: The last two rows in this table are guesses, AFAIK there's no way to test a closed RN app on dev, and if you leave your phone locked for long enough for the server to realize you're not connected to Pusher, XCode will also kill the metro process.

App State Notification Displays? Saved to Onyx? Tapping opens to correct report?
Foreground, current report No Yes n/a
Foreground, another report Yes ✅ Yes Yes ✅
Background, pusher connected Yes ✅ Yes Yes ✅
Background, pusher not connected Sometimes 🟡 No ❌ Yes
Phone locked, pusher connected Yes ✅ Yes Yes ✅
Phone locked, pusher not connected Sometimes 🟡 No ❌ Yes
Closed Sometimes 🟡 No ❌ Yes

So overall, it's a step in the right direction, but still not perfect. From what I can tell, the reason we sometimes 🟡 display notifications is that, when backgrounded, the client disconnects from Pusher without telling the server. Then there is ~5 minutes when the client is not connected via Pusher, but the server still thinks it is. So the server is not sending push notifications.

Still haven't figured out the best solution here. Also, I discovered a bug in my current implementation that seems to stem from this problem:

  1. Background the app, then wait some undetermined amount of time for the client to disconnect from Pusher.
  2. Then send a message to this user. Since the server thinks Pusher is still connected, no push notification is displayed.
  3. A few minutes later, send another message to this user. By this time, the server has figured out that the client is not connected, so it sends a push notification.
  4. Tap on the push notification, and it will open the correct chat. Pusher will reconnect, and then your messages will populate. However, local notifications will display (out-of-order) for those messages that were went while the server thought the client was still connected.

I think if I fix this bug then we could merge this, because it's not making anything worse (at least not on iOS).

Some things that aren't clear to me are: What causes the client to disconnect? Can we prevent that entirely? Can we notify the server before it happens?

@roryabraham
Copy link
Contributor Author

Oh yeah, another bug I've found:

When opening a chat from a push notification or local notification, the chat does not mark as read unless you leave the chat and come back. Seems weird, but is a completely separate problem. Can maybe squeeze a fix in without causing a regression in the unread indicators/unread badge count.

@roryabraham
Copy link
Contributor Author

Also iOS E2E tests are failing 😞

@roryabraham
Copy link
Contributor Author

roryabraham commented Jun 21, 2021 •
edited
Loading

Also – this will blast the user with local "push" notifications, even if the user has E.cash open on web/desktop. Brought this up in the parent issue, but imo that should also block this PR

@roryabraham
Copy link
Contributor Author

I think my previous understanding of the problem may have been wrong – debugging shows that pusher is still connected and the callback is being executed. However, the local notification is not always displaying when the app is in the background. This might be related to zo0r/react-native-push-notification#1574

@roryabraham
Copy link
Contributor Author

react-native-push-notification library has been abandoned, so we'd probably need to use something else for local notifications. Probably notifee

@Julesssss Julesssss mentioned this pull request Jun 14, 2022
Closed
@roryabraham roryabraham deleted the Rory-BackgroundPushNotifications branch July 6, 2022 05:46
@atef1995 atef1995 mentioned this pull request Oct 23, 2023
Open
@chiragsalian chiragsalian mentioned this pull request Nov 4, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@roryabraham roryabraham

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@roryabraham